-
-
Notifications
You must be signed in to change notification settings - Fork 307
Add "$ref", numeric exclusive*, default empty string arrays to meta-schemas, standardize meta-schema formats #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is one alternative for issue json-schema-org#101. If accepted, the meta-schema change will come separately after PR json-schema-org#168 is merged.
This is an option for implementing issue json-schema-org#101, this time not just for subschemas but for all schemas including the root schema. The meta-schema changes will be added after PR json-schema-org#168 is approved, at which point the changes for this will be much simpler.
"dependencies": { | ||
"exclusiveMaximum": [ "maximum" ], | ||
"exclusiveMinimum": [ "minimum" ] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing "required": ["$ref"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For actual JSON References it's on line 14 in the "jsonReference" definition.
In this case, all we need to happen is that if "$ref" is present at all, the validation of the "schemaObject" definition needs to fail, and the negated schema here is sufficient to do that without the need for "required".
The negated schema does not need to match the "jsonReference" definition, and in fact should not because a non-string "$ref" should fail both "jsonReference" and "schemaObject".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON Schema {"properties": { "$ref": {} }}
is equal to {}
, meaning it does not enforce anything. What you actually intented to write is "not": { "required": ["$ref"] }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[EDIT: I'm probably wrong, you can skip this]
"$ref" indicates a reference when used where a subschema is expected. But it is not a keyword otherwise- this is a change between draft 4 and draft 5. This allows for objects to define properties that are literally named "$ref", which was previously impossible. This is the same as other schema keywords not being keyword within "properties" (you can have a property called "id", for instance).
So that negated schema successfully validates any object that has a "$ref" property. Which means that, once negated, any object with a "$ref" property will fail validation. Since we're using this negated schema in the non-ref schema, that is what we want. Recall that in a JSON Reference:
- The value of "$ref" MUST be a URI reference string
- All other fields in a reference MUST be ignored
So if you put a "$ref" with in an otherwise normal schema, then it is a reference and all of the other schema keywords MUST be ignored. That means that any "$ref", whether its value is a URI reference or not, must cause the normal meta-schema validation to fail (so that the reference validation that is also in the "oneOf" can succeed).
If "$ref"'s value is a URI reference string, then it will pass the reference side of the "oneOf", which is correct.
If "$ref"'s value is not a string (or not a URI reference if format validation is supported), then it will fail the reference side of the "oneOf" and fail validation overall, which is also correct. A malformed "$ref" is not a valid schema.
I'm quite sure that this schema does what I want it to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"not": { "required": [ "$ref" ] }
also works but I don't like how it reads. "cannot have a property named $ref" read better than "$ref is anti-required" to me. Obviously preferences will vary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JanTvrdik wait, maybe I'm not sure... lemme think about it... gah...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that negated schema successfully validates any object that has a "$ref" property.
Yes, that is correct. It however also successfully validates any object without "$ref" property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct. It however also successfully validates any object without "$ref" property.
Yeah, that's why I reversed myself :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it should be "not": { "required": [ "$ref" ] }
here, correct?
} | ||
}, | ||
"required": [ "$ref" ] | ||
}, | ||
"schemaArray": { | ||
"type": "array", | ||
"minItems": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@handrews I mean here re comment in the list of PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minItems should be removed and ref should be added, as above in hyper-schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure I follow but I need to re-do this whole series of commits anyway so I will double check as I do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right- you meant in the unchanged part of the schema. I was looking at the green-highlighted part and was confused. Yes, I'll take a look at that.
8ed2034
to
a360b17
Compare
OK, this incorporates all feedback. Here are the individual commits (review it like this, NOT as one giant diff): Formatting standardization, tie hyper-schema's "schemaArray" to validation's: Move definitions to top but do not change anything else (more standardization): Add "$ref" for subschemas: Update for numeric exclusives, empty schema arrays, give string arrays a default of empty array: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with everything here but I can't vouch for the 3rd commit (regarding notJsonReference), I clearly don't understand hyper-schema enough today to understand it.
@Relequestual notJsonReference has nothing to do with hyper-schema. It implements this part of the core spec:
What that means is that if "$ref" is present, the object cannot be used as a regular JSON Schema. So it should not validate as one. To walk through it, let's start with the notJsonReference schema: "notJsonReference": {
"not": {
"required": [ "$ref" ]
}
} The inner schema, So, when negated, this means that non-objects fail validation (because there is no "type" restriction in the inner schema) and objects with "$ref" fail validation, regardless of the value of "$ref" (because "$ref" being present at all causes the "required" to pass). Now let's take a look at the oneOf/allOf construct: "subSchema": {
"oneOf": [
{
"allOf": [
{ "$ref": "#" },
{ "$ref": "#/definitions/notJsonReference" }
]
},
{ "$ref": "#/definitions/jsonReference" }
]
} This "oneOf" says that a subschema must either be a regular schema (the "allOf" branch) or a JSON Reference (the "#/definitions/jsonReference" branch) but not both. So consider this instance: {
"type": "object",
"description": "Lorem ipsum...",
"$ref": "#/definitions/something"
} This is both trying to be a regular schema (type and description) and a reference ($ref). Without "notJsonReference", this would validate as both a regular schema and a reference, which is wrong. Even though there's nothing structural that would otherwise disqualify this as a schema, the spec says that if "$ref" is present, the other fields MUST be ignored. So even if they look like a schema, they will not behave like one. So this instance should not validate as a regular schema. Without "notJsonReference", it would validate as a regular schema because JSON Schema does not forbid unknown properties. So we need to specifcially forbid $ref, which is what "#/definitions/notJsonReference" does. Instead, it should validate as a reference, because the value of "$ref" is a URI Reference string. And then the other fields are correctly ignored. Finally, consider {
"$ref": 42
} This should fail validation entirely. It will fail the "jsonReference" branch of the "oneOf" because 42 is not a string (much less a URI Reference). But without "notJsonReference", this would successfully validate as a regular schema, because unknown properties do not cause validation to fail. So we need "notJsonReference" to ensure that this fails the regular schema branch of the oneOf as well as the JSON Reference branch. With "notJsonReference" involved, this instance correctly fails validation entirely. To summarize: IF "$ref" is present, the instance must validate as a JSON Reference and not as anything else. If it fails to validate as a JSON Reference, it must fail validation entirely. ELSE ("$ref" is not present) it passes validation if it is otherwise a regular JSON Schema. |
This brings formatting and most of the initial field ordering in line with the Draft 05 meta-schemas proposed in the web site repo. Also ensure that hyper-schema's definition of "schemaArray" cannot get out of sync with schema's, and add the link schema.
This brings it in line with the core/validation meta-schema and with the standardized format proposed for Draft 05.
@handrews your metaschema is correct and explanation makes perfect sense. Do you see though how difficult it is for everybody to see the intention (even though @Relequestual has seen a couple of schemas) - although your schema achieves the intention you describe, it is very difficult to understand what it does. Also, I hope you notice the language you use to describe this intention (IF/THEN/ELSE - you even put two of these words in capitals, to make it clearer). This schema would have been very easy for us to understand and wouldn't have raised the question if these keywords actually existed: {
"if": { "required": ["$ref"] },
"then": { "$ref": "#/definitions/jsonReference" },
"else": { "$ref": "#" }
} And it doesn't prescribe any order of execution any more than allOf/anyOf/oneOf do. Subschemas in the latter keywords can be evaluated in any order (although all validators I know evaluate them in the order of array items and short-circuit, unless the mode is to collect all possible errors). In the same way subschemas in if/then/else can be evaluated in any order - it will not affect the final result (although I can't see why validators will not do the sensible thing here as well). This validation scenario is not constrained to meta-schema. It is very common in real world. And every time, without if/then/else you have to resort to the equivalent schema without if/then/else (see #180), as you did in meta-schema. The problem with that approach is that it is very difficult to see the intention behind this equivalent schema. If you don't want to see three "imperative looking" keywords the same can be expressed as: {
"conditional": [
{ "required": ["$ref"] },
{ "$ref": "#/definitions/jsonReference" },
{ "$ref": "#" }
]
} where the keyword "conditional" must have exactly 2 or 3 items ("else" is optional). That's how lispy languages do it (e.g., clojure). But this way or another we need a way to express it, as you can see from your own explanation. |
@epoberezkin Please don't take over PRs with other issues. |
@handrews ok. sorry. couldn't resist... Should've put the same comment in the issue. |
No worries. I read the issue first anyway and it all worked out as it was. |
@handrews This PR seems to introduce a few typos, mostly missing commas. Can this get broken up into a smaller series of PRs? There's a few things to note that would make our life easier, like I don't think boolean "exclusiveMinimum" actually depends on "minimum" existing (it just doesn't make any sense by itself). |
@awwright it's already broken into smaller commits. Just review the individual commits. If you tell me what you need changed I will apply the changes to each commit as needed. GitHub doesn't do stacked PRs at all easily. The dependency for the exclusives comes directly from the existing meta-schema and I don't want to change it. "it doesn't make any sense by itself" is exactly what that is trying to show. There's no need to take that out when it was fine before. |
This is the same approach used in the PR for Draft 05 meta-schemas in the web site repo.
Fixed various typos / missing comments /etc. Since the commit hashes changed, I have updated the links to the individual commits in #168 (comment) |
It's not just you, I'm not sure what's going on. I will take a look at it. |
{ | ||
"allOf": [ | ||
{ "$ref": "#" }, | ||
{ "$ref": "http://json-schema.org/draft/schema#/definitions/notJsonReference" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this ref be a realative pointer? We wouldn't want to make this version of the meta-schema possible to be invalid by a change at a later date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for line 62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
References to the main schema have never been relative (see the Draft 4 schema) and I'd rather not change that as part of this PR. I'm trying to do the minimal changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense. Should be logged that it should be fixed though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the numeric form of exclusiveMinimum in uniqueItems. Update the schema to take either the boolean form (with dependencies and default) or the numeric form for exclusiveMinimum and exclusiveMaximum. Also add empty array as default for string arrays. This brings the meta-schema in line with the specification.
@awwright @Relequestual I don't know what the problem was but I amended the last commit (reworded the commit message) and force-pushed and it has the right commits now: Formatting standardization, tie hyper-schema's "schemaArray" to validation's: Move definitions to top but do not change anything else (more standardization): Add "$ref" for subschemas: Update for numeric exclusives, empty schema arrays, give string arrays a default of empty array: |
As a reminder, the goal is to match the current specification and not change anything more than that.
|
Right. Thanks for explaining. That makes sense. None of these are changes, just fixes to bring the meta-schema in line with what the spec actually says. Great stuff. I'm totally happy with this! (Although we should make sure we capture the problems that exist, in new issues.) |
I keep reviewing this, but it's just too difficult to tease apart all of the things going on together, and that's holding up progress on this which is unfortunate. So I'll close this out, but we should deal with all of the issues presented here. I filed #185 to deal with the case of "exclusive*" properties. For formatting conserns, I'd like to see a particular rationale for why whitespace should be formatted the way it is. We should document these patterns in README. I noticed anyOf/allOf/oneOf still requires at least one item, so we can't remove There's a lot going on here so I'm probably missing some stuff, I'd like to comment on these individually so please let me know if there was anything else being dealt with here. |
Do you not understand how to review individual commits? I would like a yes or no answer on this. |
@handrews I'm not even sure what to say. Yes, I see the feature to review commits. But there's multiple distinct issues here, that I would like to be able to merge individually, instead of having to merge everything at-once. |
@awwright You also could have tried telling me what these "multiple distinct issues" are. With the exception of the point we're arguing in #185 I made every single change you asked for, immediately. I went on IRC since you wouldn't do it here. I walked you through everything repeatedly. I answered questions, and I changed things and updated the stack appropriately. I do all of that, and you just close it because of unspecified "issues" that you won't describe or give me a chance to fix. Everyone else who reviewed this had no trouble articulating their concerns, which I also addressed. But you just won't. Even when closing the PR you're incapable of actually articulating the real problems, which we still don't know. |
@awwright I was about to split off the exclusive* bits since you opened #185, but you didn't even come back here and ask for that, you just closed it. And even though you commented here after I opened #186 in an effort to accommodate you once you closed this PR, you didn't bother to comment on that at all. |
Should be noted that #188 fixes part of this. |
This is the corresponding change to json-schema-org/json-schema-org.github.io#57
Like that PR, is broken into several commits to make the diffs much easier to read.
It also has meta-schema changes for #77 numeric exclusives and #69 defaulting string arrays to empty arrays, which are the last bits to finish those two issues.
The commits in the sequence are:
NOTE: Various outstanding PRs will be reworked to put their meta-schema changes on top of this as soon as it is merged. In particular this will simplify and clarify both #128 and #167 .